Conversation
|
@codex review |
There was a problem hiding this comment.
Pull request overview
This PR implements a secret storage feature that allows users to securely store, manage, and audit sensitive company information like API keys, tokens, and passwords. The feature includes encrypted storage with optional master password protection, comprehensive audit logging, and company-based access control.
- Adds complete secret storage backend with entities, service, controller, and DTOs
- Implements two-layer encryption (application-level + optional master password)
- Adds database migrations for user secrets and audit logs
- Includes comprehensive unit and E2E test coverage
- Updates Docker configuration for test environment
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 26 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/test/ava-tests/non-saas-tests/non-saas-secrets-e2e.test.ts | Comprehensive E2E tests covering all secret operations including CRUD, authentication, authorization, and master password protection |
| backend/src/migrations/1763724062000-CreateSecretAccessLogEntity.ts | Database migration for audit log table with proper indexes and foreign keys |
| backend/src/migrations/1763724061000-CreateUserSecretEntity.ts | Database migration for user secrets table with encryption support and company-level scoping |
| backend/src/entities/user-secret/user-secrets.service.ts | Core service implementing secret management with encryption, access control, and audit logging |
| backend/src/entities/user-secret/user-secrets.service.spec.ts | Unit tests for the secrets service covering all major operations and error cases |
| backend/src/entities/user-secret/user-secret.module.ts | NestJS module definition registering the secret storage components |
| backend/src/entities/user-secret/user-secret.entity.ts | TypeORM entity for user secrets with relationships and indexes |
| backend/src/entities/user-secret/user-secret.controller.ts | REST API controller exposing secret management endpoints |
| backend/src/entities/user-secret/application/dto/update-secret.dto.ts | DTO for updating secret values with validation |
| backend/src/entities/user-secret/application/dto/secret-list.dto.ts | DTOs for paginated secret list responses |
| backend/src/entities/user-secret/application/dto/found-secret.dto.ts | DTO for returning secret details with optional value |
| backend/src/entities/user-secret/application/dto/create-secret.dto.ts | DTO for creating secrets with validation and master password support |
| backend/src/entities/user-secret/application/dto/audit-log.dto.ts | DTOs for audit log entries and paginated responses |
| backend/src/entities/secret-access-log/secret-access-log.entity.ts | TypeORM entity for tracking all secret access events |
| backend/src/decorators/company-id.decorator.ts | Custom decorator for extracting company ID from authenticated requests |
| backend/src/app.module.ts | Registers the UserSecretModule in the main application module |
| justfile | Adds --attach flag to focus test output on backend_test container |
| docker-compose.tst.yml | Updates test environment with explicit DATABASE_URL and simplified volume mounts |
| Dockerfile | Modifies file ownership configuration for the application directory |
| SECRET_STORAGE_SPECIFICATION.md | Comprehensive specification document detailing the secret storage feature design |
Comments suppressed due to low confidence (1)
backend/test/ava-tests/non-saas-tests/non-saas-secrets-e2e.test.ts:71
- Unused variable connectionId.
const connectionId = JSON.parse(createdConnection.text).id;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .set('Content-Type', 'application/json') | ||
| .set('Accept', 'application/json'); | ||
|
|
||
| const connectionId = JSON.parse(createdConnection.text).id; |
There was a problem hiding this comment.
The variable connectionId is created but never used in this test or any other tests in the file. Each test creates a connection but doesn't use the connectionId for any assertions or operations. Consider removing these unused variable declarations throughout the test file (lines 71, 107, 138, 170, 202, 228, 254, 277, 312, 334, 361, 392, 426, 455, 490, 516, 550, 573, 600, 623) to improve code maintainability.
| const connectionId = JSON.parse(createdConnection.text).id; | |
| // Removed unused connectionId variable |
| Post, | ||
| Put, | ||
| Query, | ||
| UseGuards, |
There was a problem hiding this comment.
Unused import UseGuards.
| UseGuards, |
| jest.spyOn(auditLogRepository, 'save').mockResolvedValue({} as SecretAccessLogEntity); | ||
|
|
||
| const result = await service.updateSecret('user-uuid-1', 'test-secret', updateDto); | ||
|
|
There was a problem hiding this comment.
Unused variable result.
| expect(result.slug).toBe('test-secret'); | |
| expect(result.encryptedValue).toBe('new-value'); |
| .set('Content-Type', 'application/json') | ||
| .set('Accept', 'application/json'); | ||
|
|
||
| const connectionId = JSON.parse(createdConnection.text).id; |
There was a problem hiding this comment.
Unused variable connectionId.
| const connectionId = JSON.parse(createdConnection.text).id; |
| .set('Content-Type', 'application/json') | ||
| .set('Accept', 'application/json'); | ||
|
|
||
| const connectionId = JSON.parse(createdConnection.text).id; |
There was a problem hiding this comment.
Unused variable connectionId.
| .set('Content-Type', 'application/json') | ||
| .set('Accept', 'application/json'); | ||
|
|
||
| const connectionId = JSON.parse(createdConnection.text).id; |
There was a problem hiding this comment.
Unused variable connectionId.
| .set('Content-Type', 'application/json') | ||
| .set('Accept', 'application/json'); | ||
|
|
||
| const connectionId = JSON.parse(createdConnection.text).id; |
There was a problem hiding this comment.
Unused variable connectionId.
| .set('Content-Type', 'application/json') | ||
| .set('Accept', 'application/json'); | ||
|
|
||
| const connectionId = JSON.parse(createdConnection.text).id; |
There was a problem hiding this comment.
Unused variable connectionId.
| const connectionId = JSON.parse(createdConnection.text).id; |
Artuomka
left a comment
Artuomka
left a comment
There was a problem hiding this comment.
The implementation does not align with the overall project architecture.
New entities have been added that are related to existing ones, but the corresponding changes were not made to the existing entities.
|
|
||
| export class AuditLogResponseDto { | ||
| @ApiProperty({ type: [AuditLogEntryDto] }) | ||
| data: AuditLogEntryDto[]; |
There was a problem hiding this comment.
({ type: AuditLogEntryDto, isArray: true })
| @Column({ type: 'varchar', length: 4096, nullable: true }) | ||
| masterHash: string; | ||
|
|
||
| @OneToMany(() => SecretAccessLogEntity, (log) => log.secret) |
There was a problem hiding this comment.
need to add description of the inverse relation
| @PrimaryGeneratedColumn('uuid') | ||
| id: string; | ||
|
|
||
| @ManyToOne(() => CompanyInfoEntity, { onDelete: 'CASCADE' }) |
There was a problem hiding this comment.
need to add description of the inverse relation
Artuomka
left a comment
Artuomka
left a comment
There was a problem hiding this comment.
need to improve swagger DTOs, need to add inverse relations in TypeORM entities (secret.accessLogs ↔ accessLog.secret), change ForbiddenException to NotFoundException when user is not found, and add guard to endpoints, if its needed
No description provided.